Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #221: Add marginal model #426

Merged
merged 62 commits into from
Dec 3, 2024
Merged

Issue #221: Add marginal model #426

merged 62 commits into from
Dec 3, 2024

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Nov 7, 2024

Description

This PR implements the marginal model as in #221.

  • The file marginal_model-scratch.R implements a prototype outside of functions
  • This prototype is implemented into the standard structure
  • Uses primarycensored function primarycensored_lpmf which is passed into via a wrapper in marginal_model/functions.stan
    • Uses only the analytic versions of this distribution
  • Adds tests for new marginal_model

Possible steps forward here:

  • But uses non vectorised version of LPMF: want to extend to be using vectorised version of LPMF
  • Could want to extend to being able to use non analytic solution in a natural way

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

inst/cohort-scratch.R Outdated Show resolved Hide resolved
inst/cohort-scratch.R Outdated Show resolved Hide resolved
@athowes athowes changed the title Issue #221: Add cohort model Issue #221: Add marginal model Nov 14, 2024
@athowes athowes force-pushed the cohort-model branch 2 times, most recently from a415d16 to aa24b86 Compare November 15, 2024 21:04
@athowes

This comment was marked as off-topic.

@athowes

This comment was marked as off-topic.

@athowes

This comment was marked as outdated.

@epinowcast epinowcast deleted a comment from codecov bot Dec 2, 2024
@seabbs
Copy link
Contributor

seabbs commented Dec 3, 2024

@athowes there are a few bows to tie here and I am investigating the CI issues on mac and windows (it looks likes its also on main so presumably unrelated) but I think this is at the point its worth taking a look at. See https://github.com/epinowcast/epidist/actions/runs/12112515981/job/33766065135 for the same failure on main.

Updates in bullets:

  • Added a marginalised likelihood model based on primarycensored. This can be specified using as_epidist_marginal_model(). This is currently limited to Weibull, Log-Normal, and Gamma distributions with uniform primary censoring but this will be generalised in future releases. This is waiting on A lookup in R to find the stan number associated with a probability distribution primarycensored#175.
  • Added a transform_data s3 method to allow for data to be transformed for specific models. This is specifically useful for the marginal model at the moment as it allows reducing the data to its unique strata.
  • Switched over to using the marginal model everywhere
  • Added helper functions for new variables to avoid code duplication in vignettes
  • Added a heuristic for the marginal model that adapts the relative observation time to be Inf when it is much longer than the maximum delay. This is required as otherwise there are numerical instability errors from try to divide by something very near one (i.e the cdf at long observation times). In primarycensored this is a warning but here with more naive users I think it makes sense to make an informed choice and flag that that has happened.

@seabbs seabbs marked this pull request as ready for review December 3, 2024 10:23
seabbs

This comment was marked as outdated.

seabbs
seabbs previously approved these changes Dec 3, 2024
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (self-review)

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/marginal_model.R Show resolved Hide resolved
R/marginal_model.R Show resolved Hide resolved
inst/stan/latent_model/functions.stan Outdated Show resolved Hide resolved
tests/testthat/setup.R Outdated Show resolved Hide resolved
tests/testthat/test-marginal_model.R Show resolved Hide resolved
vignettes/ebola.Rmd Show resolved Hide resolved
vignettes/faq.Rmd Show resolved Hide resolved
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following comments from @athowes a follow review from me. LGTM so merging

@seabbs seabbs merged commit ce41f42 into main Dec 3, 2024
8 of 10 checks passed
@seabbs seabbs deleted the cohort-model branch December 3, 2024 13:22
@seabbs seabbs mentioned this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants